Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple lflip #335

Merged
merged 9 commits into from
Dec 19, 2024
Merged

Simple lflip #335

merged 9 commits into from
Dec 19, 2024

Conversation

jordandekraker
Copy link
Collaborator

Here we remove all instances of flipping EXCEPT for templateseg and and shape_inject (where we now apply them to the templates instead of the images)

@jordandekraker
Copy link
Collaborator Author

wetrun testing right now

@akhanf akhanf mentioned this pull request Dec 19, 2024
@jordandekraker
Copy link
Collaborator Author

My wetrun test seems to have passed (I also modified the test to handle the new manualseg)

Note that the templateseg neonate pipeline doesn't look great (the registration seems to sometimes place the hippocampus in the wrong place), and it may even have performed better using nnunet.

I think this PR is ready for merge!

@akhanf
Copy link
Member

akhanf commented Dec 19, 2024

Ok great - I'm at my computer now so I'll have a quick look now before merging

@@ -356,37 +356,28 @@ xfm_identity_itk: resources/etc/identity_xfm_itk.txt

#templates enabled for template-based segmentation are here
# TODO: should also perhaps include modalities avaialble, and any custom crop_native_res settings
template_based_segmentation:
template_available_hemis:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put this inside the template_files dict to keep things together? I can do that in my edits..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just made that edit -- generally trying to keep things hierarchical in the config (will make anotehr issue to do this en-masse, as the config right now is really messy with so many different top-level keys (which don't remain sorted in the nice way that we have them when deployed to an output folder)..

- adds a hemi_wildcards field instead
@akhanf
Copy link
Member

akhanf commented Dec 19, 2024

this is good to merge from my end -- I think you could do a Squash and merge instead, to keep the history clean (since all the commits can conceptually fit in a single commit)

@jordandekraker jordandekraker merged commit 3d0a60c into dev-v2.0.0 Dec 19, 2024
4 checks passed
@jordandekraker jordandekraker deleted the simpleLflip branch December 19, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants